Fix lint #1529

DonggeLiu · 2022-10-19T04:26:03Z

Based on and required by #1526.

This PR fixes new lint errors from the latest pylint, which is required as #1526 upgrades Python to 3.10.
I reckon I have fixed all meaningful errors in the FuzzBench framework, not sure if the rest is worth fixing or could be ignored.

The rest lint errors are:

From individual fuzzers (e.g., fuzzers/*);
In the database directory (i.e., database/*);
trivial errors:
- conventional naming (e.g., df doesn't conform to snake_case naming style, but is common for pandas.dataframe).
- too-few-public-methods (e.g., Too few public methods).
- Using the global statement.
- Improper Consider using 'with' for resource-allocating operations, e.g., with Popen, urlopen.
- Improper Using open without explicitly specifying an encoding, e.g., when opening a binary file.

Could you please take a look at the rest errors and let me know which one is worth fixing?
Otherwise, I will find a way to let pylint ignore them in the future.

DonggeLiu · 2022-10-19T04:28:09Z

It seems you will need to pull this branch and run make presubmit locally to see the errors.
Do not trust the 'All checks have passed' message here, there are quite many errors.

jonathanmetzman

Thanks for this.
i think we will need to discuss this offline.
I have some concerns. Mainly, why update .pylintrc?

jonathanmetzman · 2022-10-19T18:34:19Z

.pylintrc

-# paths.
-#
-# Files under alembic are generated and are nonconforming.
-ignore=alembic


Why get rid of stuff like this?

This pylintrc was generated automatically with the latest version of pylint, I am working on manually merging our old settings back so that we can have:

The default settings of the latest pylint

The same setting from us in the past.

jonathanmetzman · 2022-10-19T18:34:42Z

.pylintrc

-jobs=0
+# number of processors available to use, and will cap the count on Windows to
+# avoid hangs.
+jobs=1


No we don't care about windows.

jonathanmetzman

Most changes ok to land whenever (as long as CI passes without the .pylintrc change)

jonathanmetzman · 2022-10-19T18:38:14Z

analysis/plotting.py

 import numpy as np
 import Orange
 import seaborn as sns

+from matplotlib import colors


Did google's styleguide get rid of alphabetical order imports? It looks like it might have.

Yep, I remember pylint specifically requested me to put this at the top (which also surprised me), because it is an external import.

jonathanmetzman · 2022-10-19T18:43:23Z

experiment/measurer/measure_manager.py

@@ -122,15 +122,15 @@ def measure_loop(experiment: str,
        set_up_coverage_binaries(pool, experiment)
        # Using Multiprocessing.Queue will fail with a complaint about
        # inheriting queue.
-        q = manager.Queue()  # pytype: disable=attribute-error
+        manager_queue = manager.Queue()  # pytype: disable=attribute-error


call this multiprocessing_queue

jonathanmetzman · 2022-10-19T18:44:26Z

experiment/measurer/test_run_coverage.py

@@ -13,8 +13,6 @@
 # limitations under the License.
 """Tests for run_crashes.py."""

-# pylint: disable=no-self-use


We may need to keep these for now until we update pylintrc

Thanks for this. i think we will need to discuss this offline. I have some concerns. Mainly, why update .pylintrc?

I updated .pylintrc mainly because I updated pylint, which is because we updated python.
This .pylintrc is automatically generated with the latest version of pylint, which I presume contains many new settings (e.g., some default settings may have been changed to optional).
I am also working on manually merging some of our old changes to here, e.g. ignore=alembic

Happy to discuss more on this, as I am not certain which settings should be kept.

Just come across another reason to update .pylintrc: I found 35 deprecated and 5 unknown pylint messages in our old .pylintrc that cause errors with the new pylint.

I updated .pylintrc mainly because I updated pylint, which is because we updated python.

I think (not sure) this isn't the first time we've upgraded pylint, we usually don't regenerate the entire file.

This .pylintrc is automatically generated with the latest version of pylint, which I presume contains many new settings (e.g., some default settings may have been changed to optional).

I think if we can get away without changing pylintrc now we should, the lint upgrade is incidental, to upgrading python, so it's fine if it's minimal. We can worry about being consistent later.

DonggeLiu · 2022-10-20T04:16:09Z

Merged our old .pylintrc into the new one.
Fixed almost all lint errors in the framework, except a few (which we can either ignore or need to discuss).
Errors of each fuzzers are not fixed yet, maybe that depends on the contributor of them?

jonathanmetzman · 2022-10-20T13:00:14Z

Merged our old .pylintrc into the new one. Fixed almost all lint errors in the framework, except a few (which we can either ignore or need to discuss). Errors of each fuzzers are not fixed yet, maybe that depends on the contributor of them?

No i think we need to fix these :-(

DonggeLiu · 2022-10-20T21:26:46Z

Merged our old .pylintrc into the new one. Fixed almost all lint errors in the framework, except a few (which we can either ignore or need to discuss). Errors of each fuzzers are not fixed yet, maybe that depends on the contributor of them?

No i think we need to fix these :-(

Sounds like another TODO for me today 🔖

DonggeLiu · 2022-10-21T00:23:20Z

common/filesystem.py

@@ -42,6 +42,7 @@ def recreate_directory(directory, create_parents=True):

 def write(path, contents, open_flags='w'):
    """Opens file at |path| with |open_flags| and writes |contents| to it."""
+    # pylint: disable=unspecified-encoding
    with open(path, open_flags) as file_handle:


Not sure if there is a better way to fix this:
We cannot simply add encoding='utf-8' as the path can be a binary file.

I think this function is probably a bad idea, maybe just get rid of it.

OK, I left a todo on it for another PR as it is involved in several other functions.

DonggeLiu · 2022-10-21T00:32:19Z

fuzzers/utils.py

-    # Matches UBSan features enabled in OSS-Fuzz.
-    # See https://github.com/google/oss-fuzz/blob/master/infra/base-images/base-builder/Dockerfile#L94
+    # Matches UBSan features enabled in OSS-Fuzz. See
+    # https://github.com/google/oss-fuzz/blob/master/infra/base-images/base-builder/Dockerfile#L94


.pylintrc can ignore long URL lines, but its regex pattern needs the line to start with http/https.
Would you prefer me to fix this error like this (i.e., forcing long URL lines in code to follow the existing pattern in pylintrc), or change the pattern in .pylintrc (e.g., ignore all lines that contain a URL like the old version)?

DonggeLiu · 2022-10-21T04:50:17Z

I reckon all errors from make presubmit are fixed now: )

DonggeLiu · 2022-10-23T22:37:55Z

@jonathanmetzman: I reckon this is ready to be merged into #1526, please let me know what you think : )

jonathanmetzman · 2022-10-24T15:22:00Z

Why not merge it here?
Do you know why CI isn't running anymore? I wonder if it's from adding new fuzzers to CI?

jonathanmetzman

LGTM thanks

jonathanmetzman · 2022-10-24T15:22:40Z

.github/workflows/fuzzers.yml

+          - aflpp_random_no_favs
+          - aflpp_random_wrs
+          - aflcc
+          - aflpp_random_wrs_rf_rp


Don't add these please, I think it's breaking CI. Remove them please. CI will fail but that's fine IMO

jonathanmetzman · 2022-10-24T15:23:42Z

common/filesystem.py

@@ -42,6 +42,7 @@ def recreate_directory(directory, create_parents=True):

 def write(path, contents, open_flags='w'):
    """Opens file at |path| with |open_flags| and writes |contents| to it."""
+    # pylint: disable=unspecified-encoding
    with open(path, open_flags) as file_handle:


I think this function is probably a bad idea, maybe just get rid of it.

jonathanmetzman · 2022-10-24T15:29:25Z

experiment/measurer/measure_manager.py

@@ -145,8 +147,8 @@ def measure_loop(experiment: str,
    logger.info('Finished measure loop.')


-def measure_all_trials(experiment: str, max_total_time: int, pool, q,
-                       region_coverage) -> bool:  # pylint: disable=invalid-name
+def measure_all_trials(experiment: str, max_total_time: int, pool,


Can you remove this pylint directive?

jonathanmetzman · 2022-10-24T15:29:46Z

experiment/measurer/measure_manager.py

-            continue
+            member_file_handle = tar.extractfile(member)
+            if not member_file_handle:
+                logger.info('Failed to get handle to %s', member)


nit end thsi with a period.

jonathanmetzman · 2022-10-24T15:33:23Z

fuzzers/eclipser/fuzzer.py

@@ -94,7 +94,8 @@ def eclipser(input_corpus, output_corpus, target_binary):
    if os.listdir(input_corpus):  # Specify inputs only if any seed exists.
        command += ['-i', input_corpus]
    print('[eclipser] Run Eclipser with command: ' + ' '.join(command))
-    subprocess.Popen(command)
+    with subprocess.Popen(command):
+        pass


Can you disable this warning just for Popen? If you can let's just do this in .pylintrc (this is kind of burdensome for files in fuzzers/)

jonathanmetzman · 2022-10-24T15:33:46Z

fuzzers/gramatron/fuzzer.py

@@ -52,7 +52,7 @@ def build():  # pylint: disable=too-many-branches,too-many-statements
        raise RuntimeError('Unsupported benchmark, unavailable grammar')
    dest = os.path.join(os.environ['OUT'], 'grammar.json.gz')
    shutil.copy(copy_file, dest)
-    os.system("gzip -d '%s'" % dest)
+    os.system(f"gzip -d '{dest}'")


nit: single quotes on outside.

jonathanmetzman · 2022-10-24T15:36:56Z

.pylintrc

-known-third-party=enchant
+# This flag controls whether inconsistent-quotes generates a warning when the
+# character used as a quote delimiter is used inconsistently within a module.
+check-quote-consistency=no


What if you change this to "yes"

It finds 299 new errors, 272 of which are from different fuzzers' fuzzer.py : )

jonathanmetzman

ACtually, don't land this without removing those fuzzers from CI please.

jonathanmetzman · 2022-10-24T15:56:35Z

btw I think CI builds still won't run after you undo your additions, it's because of #1536 that it's failing right now.

DonggeLiu · 2022-10-24T23:56:47Z

Why not merge it here?

I consider this as a part of upgrading Python in #1526, as new Python (and new pylint) has new format/lint requirements.
It is a patch to fix the CI failure in #1526.

This reverts commit dbe5889.

DonggeLiu · 2022-10-25T01:59:15Z

btw I think CI builds still won't run after you undo your additions, it's because of #1536 that it's failing right now.

OK, I've reverted the commit.

DonggeLiu · 2022-10-25T02:19:20Z

Do you know why CI isn't running anymore? I wonder if it's from adding new fuzzers to CI?

I am wondering about that, too.
From the history of merged PRs, I can see that #1517 is the last PR that runs all CIs, and then my #1525 is the first PR that should have run all CIs but did not.
So I guess it must be one (or more) of the PRs in between, i.e. {#1517, #1518, #1519, #1529, #1522, #1523,#1524}.

jonathanmetzman · 2022-10-25T18:22:35Z

I think if you pull CI will run again

DonggeLiu · 2022-10-26T06:52:50Z

I think if you pull CI will run again

Thanks! It looks alright now.
Would you mind me merging this back to #1526 and fixing the fuzzers' build errors (by removing the corresponding benchmarks, etc.)?
I want to make this PR mainly about fixing lint and other presubmit errors.

DonggeLiu · 2022-10-26T06:54:25Z

I think if you pull CI will run again

And by the way, why would the fuzzers in #1537 disable all CI build checks?

jonathanmetzman · 2022-10-26T23:11:35Z

I think if you pull CI will run again

And by the way, why would the fuzzers in #1537 disable all CI build checks?

Github must have some limit that we run into silently 🤦

DonggeLiu · 2022-10-27T00:03:49Z

Github must have some limit that we run into silently 🤦

Yeah, I guess we do have a lot of build tests in CI...
OK, I will fix the build issues in #1526 first, then come back and merge this to #1526 as discussed.

DonggeLiu · 2022-11-23T12:21:17Z

@jonathanmetzman : Shall I merge this back to the base branch? Thanks!

jonathanmetzman

lgtm

jonathanmetzman · 2022-11-23T13:26:20Z

@jonathanmetzman : Shall I merge this back to the base branch? Thanks!

sure!

Fixes #1594: 1. Revert a previous lint fix (in #1529) to avoid using `with` on subprocesses, as the context manager made local experiments fail to launch runner instances. 2. Simplify the previous return statement, which was true by definition.

DonggeLiu added 7 commits October 18, 2022 12:34

Update pylintrc

766c155

Fixing lint-1

21eb884

Fix lint-2

a65abf4

Fix lint-3

b72f5df

Fix lint-4

5a0c56b

Fix lint-5

82f9967

Fix lint-6

d1a935a

DonggeLiu requested a review from jonathanmetzman October 19, 2022 04:26

DonggeLiu mentioned this pull request Oct 19, 2022

Upgrade base images, benchmark images, and Python. #1526

Merged

jonathanmetzman reviewed Oct 19, 2022

View reviewed changes

DonggeLiu added 6 commits October 20, 2022 13:55

Manually merge pylintrc

f91a7b2

Fix lint: Long url

152d85a

Fix lint: long lines

fbf0929

Fix lint: using with

7c58854

Fix lint: lint comment

16618a9

Merge branch 'upgrade-base-image' into fix_lint

4d5dec3

Fix naming

ad05bef

Ignore/fix lint in framework

5bb981c

DonggeLiu commented Oct 21, 2022

View reviewed changes

DonggeLiu added 5 commits October 21, 2022 11:46

Leftover lint errors

939dfbc

Minor format change

1012bda

Ignore temporary fuzzers

2323d97

Fix lint-7

967550b

Fix lint: URL

dcef066

DonggeLiu requested a review from jonathanmetzman October 23, 2022 22:36

jonathanmetzman approved these changes Oct 24, 2022

View reviewed changes

jonathanmetzman requested changes Oct 24, 2022

View reviewed changes

A todo

0013e16

DonggeLiu added 4 commits October 25, 2022 11:10

Remove redundant pylint comments

6f3f16a

End log with .

e4cb11f

Unify quotes

7a412cc

Revert "Add fuzzers to fuzzer list"

02fe2a8

This reverts commit dbe5889.

DonggeLiu requested a review from jonathanmetzman October 25, 2022 06:22

Merge branch 'master' into fix_lint

71ff5c6

jonathanmetzman approved these changes Nov 23, 2022

View reviewed changes

Merge branch 'upgrade-base-image' into fix_lint

30ab028

DonggeLiu merged commit 2ba4957 into upgrade-base-image Nov 24, 2022

DonggeLiu deleted the fix_lint branch November 24, 2022 21:32

DonggeLiu mentioned this pull request Jan 9, 2023

Revert prev lint fix #1604

Merged

Fix lint #1529

Fix lint #1529

Conversation

DonggeLiu commented Oct 19, 2022

DonggeLiu commented Oct 19, 2022

jonathanmetzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanmetzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonggeLiu commented Oct 20, 2022 • edited Loading

jonathanmetzman commented Oct 20, 2022

DonggeLiu commented Oct 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonggeLiu Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

DonggeLiu commented Oct 21, 2022

DonggeLiu commented Oct 23, 2022

jonathanmetzman commented Oct 24, 2022

jonathanmetzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanmetzman left a comment

Choose a reason for hiding this comment

jonathanmetzman commented Oct 24, 2022

DonggeLiu commented Oct 24, 2022

DonggeLiu commented Oct 25, 2022

DonggeLiu commented Oct 25, 2022 • edited Loading

jonathanmetzman commented Oct 25, 2022

DonggeLiu commented Oct 26, 2022

DonggeLiu commented Oct 26, 2022

jonathanmetzman commented Oct 26, 2022

DonggeLiu commented Oct 27, 2022

DonggeLiu commented Nov 23, 2022

jonathanmetzman left a comment

Choose a reason for hiding this comment

jonathanmetzman commented Nov 23, 2022

DonggeLiu commented Oct 20, 2022 •

edited

Loading

DonggeLiu Oct 21, 2022 •

edited

Loading

DonggeLiu commented Oct 25, 2022 •

edited

Loading